Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not wait for on-event script to finish when triggering them #3075

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Nov 27, 2024

on-event scripts don't have output so waiting for them to finish is not necessary.
Also modifying the max duration timeout to 5mins for on-event script

How to test:

  • modify your on-event script to sleep. Ex:
    const waitTime = 10;
    for (let i = 0; i < waitTime; i++) {
        await nango.log(`Countdown: ${waitTime - i}`);
        await new Promise((resolve) => setTimeout(resolve, 1000));
    }
  • create/delete a connection and then check in the logs dashboard that the script is still running and you didn't have to wait for it

@TBonnin TBonnin requested a review from a team November 27, 2024 18:59
Copy link

linear bot commented Nov 27, 2024

retry: { count: 0, max: 0 },
timeoutSettingsInSecs: {
createdToStarted: 30,
startedToCompleted: 5 * 60,
Copy link
Collaborator Author

@TBonnin TBonnin Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like all our task timeouts, this value is arbitrary. 5 mins is very generous imho

Copy link
Contributor

@nalanj nalanj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran it, looks good!

on-event scripts don't have output so waiting for them to finish is not
necessary
Also modifying the max duration timeout to 5mins for on-event script
@TBonnin TBonnin force-pushed the tbonnin/nan-2219/on-event-script-no-wait branch from 39e4068 to f83317f Compare November 27, 2024 21:02
@TBonnin TBonnin enabled auto-merge (squash) November 27, 2024 21:02
@TBonnin TBonnin merged commit 5a15445 into master Nov 27, 2024
20 checks passed
@TBonnin TBonnin deleted the tbonnin/nan-2219/on-event-script-no-wait branch November 27, 2024 21:09
@@ -52,8 +52,6 @@ export async function preConnectionDeletion({
});
if (res.isErr()) {
await logCtx.failed();
} else {
await logCtx.success();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where you success() now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in that handleOnEventSuccess function, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, might be that I must have missed it 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants